-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Enable runtime async for all libraries test projects #123448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enables the runtime async preview feature for all .NET 11+ library test projects, with appropriate exclusions for scenarios where it's not supported.
Changes:
- Add conditional PropertyGroup to enable runtime async feature for compatible test projects
- Remove trailing whitespace on line 92
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
|
@jtschuster Thanks! Anything else? |
jtschuster
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but not sure if there are any other outerloop tests we should run before merging?
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
|
/ba-g tests already ran |
| and '$(TestNativeAot)' != 'true' | ||
| and '$(TestReadyToRun)' != 'true' | ||
| and '$(UseNativeAOTRuntime)' != 'true' | ||
| and '$(TargetOS)' != 'browser' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effectively makes all libraries test projects target platform specific. Libraries test projects are target platform neutral by default. When they are target platform specific, it needs to be explicitly declared in the project file.
For example, System.Net.WebSockets.Client.Tests is conditionally compiled based on TargetOS==browser https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj#L91 and thus it requires browser-specific TFM https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj#L7
This change is going to break inner loop experiences that depend on incremental builds at minimum.
cc @dotnet/area-infrastructure-libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that if I built the System.Net.WebSockets.Client.Tests with /p:RuntimeFlavor=CoreCLR it would not run with Mono VM in the same artifacts folder ? For me that never worked because Mono and CoreCLR don't coexist well in artifacts already. I never explored the details of why. I have separate checkout directories (most of the time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about switching between platforms, like CoreCLR on windows and CoreCLR on browser. Switching platforms like this used to work fine, with CoreCLR at least.
Mono and CoreCLR don't coexist well in artifacts already
Right, switching between Mono and CoreCLR does not work with incremental builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm still not getting it -- what's the scenario where this could be broken? You compile the libraries tests for windows, then move it over to Linux and run the same test assembly there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, you compile a library test project (that does not have browser TFM) for Windows, and then you compile the same project for browser in the same enlistment. The re-compilation for browser will be skipped assuming the incremental build works as expected.
You can see the problem by looking at the binary paths under artifacts. For example, if you compile src\libraries\Microsoft.Extensions.DependencyInjection\tests\DI.Tests, the test binary path is going to be artifacts\bin\Microsoft.Extensions.DependencyInjection.Tests\Debug\net11.0\Microsoft.Extensions.DependencyInjection.Tests.dll. The platform is not mentioned in the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that '$(TargetOS)' != 'browser' condition doesn't need to be there because '$(RuntimeFlavor)' != 'Mono' is enough ? Because the coreCLR interpreter (browser) can do asyncv2, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I understand now. Basically, because test assemblies are libraries, they do not get RID-targeting by default, meaning that all RIDs basically collapse together into a single path and MSBuild does not re-evaluate.
I'm not sure I have a great way to fix this immediately. The obvious answer, that I'm working on right now, is moving the tests to xunit3 and making them Exe projects instead of libraries. This would implicitly give everything RID targeting and should solve the problem.
Unfortunately I've also hit some test failures after moving to xunit3 that I'm still debugging with Brad, so the PR isn't ready yet.
I will see if I can come up with a cheap intermediate fix, like touching a file with the name of the TFM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect that we will have the same problem with enabling runtime async for shipping libraries. You won't get away with this shortcut there. It may be better to spend time on solving that (and then retrofit the solutions to tests if it is still relevant).
No description provided.